Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(exex): WAL handle #11266

Merged
merged 8 commits into from
Sep 27, 2024
Merged

feat(exex): WAL handle #11266

merged 8 commits into from
Sep 27, 2024

Conversation

shekhirin
Copy link
Collaborator

Towards #11261

WalHandle doesn't have any methods yet, this PR just makes it clonable and passes it to the notifications structs

@shekhirin shekhirin added C-enhancement New feature or request A-exex Execution Extensions labels Sep 26, 2024
pub struct BlockCache {
/// A mapping of `File ID -> List of Blocks`.
///
/// For each notification written to the WAL, there will be an entry per block written to
/// the cache with the same file ID. I.e. for each notification, there may be multiple blocks
/// in the cache.
files: BTreeMap<u64, VecDeque<CachedBlock>>,
files: Arc<RwLock<BTreeMap<u64, VecDeque<CachedBlock>>>>,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will likely be removed, i don't think that we need an ordered mapping anymore, according to #11202

@shekhirin shekhirin marked this pull request as ready for review September 26, 2024 19:51
@shekhirin shekhirin marked this pull request as draft September 26, 2024 19:51
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some questions

crates/exex/exex/src/wal/mod.rs Show resolved Hide resolved
crates/exex/exex/src/wal/cache.rs Outdated Show resolved Hide resolved
Comment on lines 14 to 15
#[derive(Debug, Clone)]
pub struct BlockCache {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this type now has two locked fields.
arent those two fields in sync, meaning files and blocks have the same entries?
not obvious from reading the docs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previously we've deleted the notifications from WAL by walking them in an order they were inserted, so we needed a BTreeMap indexed by the file ID

we won't need it anymore and the files will be removed https://github.com/paradigmxyz/reth/pull/11266/files#r1777654333

basically, having a mapping of blocks (either hashes to file IDs, or numbers to file IDs) is enough for our new design #11202 because we don't rely on the order of files anymore and walk by hashes

@@ -310,6 +311,8 @@ pub async fn test_exex_context_with_chain_spec(
components.provider.clone(),
components.components.executor.clone(),
notifications_rx,
// TODO(alexey): do we want to expose WAL to the user?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think so

@@ -96,14 +106,6 @@ impl<Node: FullNodeComponents + Clone> ExExLauncher<Node> {
// spawn exex manager
debug!(target: "reth::cli", "spawning exex manager");
// todo(onbjerg): rm magic number
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated, outdated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated, #11277

Base automatically changed from alexey/wal-block-cache-parent-hash to main September 27, 2024 08:20
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pending parking lot semaphores

@shekhirin shekhirin added this pull request to the merge queue Sep 27, 2024
Merged via the queue into main with commit 6722124 Sep 27, 2024
36 checks passed
@shekhirin shekhirin deleted the alexey/wal-handle branch September 27, 2024 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-exex Execution Extensions C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants